Include anonymous functions in stacktraces #1508
Conversation
There was a problem hiding this comment.
Wow, nice work @jugglinmike! This looks really good. Could you have a look at why tests are failing though?
lib/serialize-error.js
Outdated
| if (typeof error.stack === 'string') { | ||
| retval.summary = error.stack.split('\n')[0]; | ||
| } else { | ||
| retval.summary = typeof error === 'function' ? |
There was a problem hiding this comment.
What's the origin of this typeof error === 'function' check?
There was a problem hiding this comment.
Yeah, this seemed a little hinky to me, too. I implemented that to satisfy an existing test which looks pretty intentional. It was introduced here: #206 . Maybe @jamestalmage can give us some perspective.
There was a problem hiding this comment.
The code comment seems informative: https://github.com/avajs/ava/blob/15bd79402c55468a4b13aedceba503ee26e964a1/lib/serialize-value.js#L11:L12
This is no longer applicable I think. We catch and wrap all errors from tests, so it's only uncaught exceptions and unhandled rejections that are passed directly to serialize-error. And right now we don't even guard against throw null. Let's remove this edge-case and we can open a follow-up for handling non-Error exceptions / rejections.
There was a problem hiding this comment.
The test demonstrates a case where a function value is thrown and not caught. Removing the test and this condition would prevent Ava from displaying the source of the function in such cases.
Granted, it seems extremely rare for a function value to be thrown in the first place. I just want to make sure we're on the same page before proceeding. Can you verify?
There was a problem hiding this comment.
Yes let's get rid of this check.
The code needs to be more defensive here but we'll handle that in a follow-up issue.
test/helper/error-from-worker.js
Outdated
|
|
||
| const serializeError = require('../../lib/serialize-error'); | ||
|
|
||
| module.exports = function (err, {type, file, stack} = {}) { |
There was a problem hiding this comment.
We're targeting Node.js 4 still, so this destructuring won't work unfortunately.
Perhaps just use Object.assign() without checking for values?
Sure thing. This was a tricky issue, but I think I have it fixed over at #1524 |
jugglinmike
force-pushed
the
include-anonymous-functions-in-stacktraces
branch
from
9f75352 to
956cdc6
Compare
Some functions are more anonymous than others. Node does a pretty
good job of naming functions like this:
```js
const fn = () => { throw "whoops!" } // <-- gets named `fn` in stacktraces
```
But in some cases a function genuinely has no name, e.g.
```js
const fn = () => () => { throw "whoops!" } // <-- inner function has no name!
```
This means we get stacktraces like this:
```
whoops!
path/to/file.js:1:1
Test.t (test.js:1:1)
```
Before this commit, the regex in `extract-stack.js` was not anticipating
stacktraces of this form, and would simply filter out the anonymous functions.
This made my life a little more awkward, as I could only see the failing test
line and the error, not the code that was actually erroring.
This commit adds another regex to `extract-stack.js` which matches on anonymous
function stacktrace lines too.
jugglinmike
force-pushed
the
include-anonymous-functions-in-stacktraces
branch
from
956cdc6 to
51cf610
Compare
Incorporate review feedback for the prior commit. Additionally, update test fixture file to satisfy the project's linting rules.
When the value of an uncaught exception is a function, AVA previously displayed the source of that function in the error report. During the review process for an unrelated change, this condition was found to be too rare to justify the maintenance overhead. Do not attempt to display the source of function values in these cases.
jugglinmike
force-pushed
the
include-anonymous-functions-in-stacktraces
branch
from
51cf610 to
9112346
Compare
|
Okay, I've pushed up another commit to remove that check. Since it reflects an intentional but subjective change, I'd prefer if we could include it in a standalone commit. That will help future contributors understand that the change in behavior was not a regression. There have been some recent commits to master...bocoup:include-anonymous-functions-in-stacktraces-orig-2 |
|
Hi @jugglinmike, I haven't forgotten about you 😄 Glad to see the tests are passing, I'm hoping to merge this later this week. |
|
No worries; I know how things get with maintaining big FOSS projects like this. Take your time, and thanks for keeping me in the loop! |
|
Thank you @jugglinmike! There should be a new release out within the next few days. |
|
Great! And thank you! |
This patch corrects Ava's reporting of stack traces which contain frames of anonymous functions. See pull request gh-1313 for the original problem description and steps to reproduce.
@neoeno originally submitted that patch in March of this year. The thread has been inactive for the 5 months since the initial review phase. This pull request is an attempt to implement that feedback and fix the bug.
Most notably, this involved implementing @novemberborn's recommendation:
To fully accommodate this change, I've updated the tests for Ava's reporters to use the internal
serialize-errormodule. This makes them somewhat more like integration tests, but this was already true to a certain extent--previously, some tests relied on the internalbeautify-stackmodule.We could update the tests to explicitly construct the errors as we expect them to result from the serialization process. Although that would tend to isolate the tests, it might also make them harder to maintain (as any change to the serialization format would have to be reflected in the tests themselves). For this initial draft, I opted for this factoring because the output of
serialize-errorseems like a pretty strong contract to depend on.As part of the discussion of gh-1313, @sindresorhus created
extract-stack, a new npm module. For reasons shared in the discussion that followed, that module is not suitable in its current form. Because of the similar-sounding functionality between modules (beatify-stack,extract-stack,clean-stack, andstack-utils), I have instead opted to renameextractStackto the slightly more preciseextractFrames. Due to the re-factoring described above, I was also able to move the function definition into thebeautify-stackmodule itself, which I believe further limits the ambiguity between module responsibilities. To shore things up a little more, I've included some documentation forbeautify-stackto describe its intended behavior along with an example.